Skip to content

PHPORM-325 Add getViews and categorize table types #3327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Apr 18, 2025
Merged

Conversation

masterbater
Copy link
Contributor

@masterbater masterbater commented Mar 24, 2025

Fix #3320
This fixes db:show and migrate:fresh for database that has virtual view type collections

  • add Connection::getViews()
  • php artisan db:show --views
  • fix collection fields introspection with qualified name

Checklist

  • Add tests and ensure they pass

@masterbater masterbater requested a review from a team as a code owner March 24, 2025 12:28
@masterbater masterbater requested a review from jmikola March 24, 2025 12:28
@masterbater
Copy link
Contributor Author

Laravel has

php artisan db:show  --views
$views = Schema::getViews();

To follow laravel convention Ill adjust and add getViews

@masterbater masterbater changed the title feat: exclude system.* collections feat: exclude system.* collections, add getViews Mar 24, 2025
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds perfect to me. A few minor comments.

@masterbater masterbater requested a review from GromNaN March 24, 2025 21:08
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the corrections.
@jmikola could you also review please.

foreach ($db->listCollections() as $collectionInfo) {
$collectionName = $collectionInfo->getName();

// Skip system collections
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The system collections are actual collections. In the case of system.profile, that might actually be something an application would want to query.

I'm not certain it makes sense to exclude these from all output through the integration.

'schema_qualified_name' => $db->getDatabaseName() . '.' . $collectionName,
'size' => null,
'comment' => null,
'collation' => null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is actually relevant, the information should be available via the options field for each collection info result. See: listCollections

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked in PHPORM-326

'name' => $collectionName,
'schema' => $db->getDatabaseName(),
'schema_qualified_name' => $db->getDatabaseName() . '.' . $collectionName,
'size' => null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is relevant, the information can be provided via $collStats (MongoDB 6.2+). I'm not sure that'd be worth the overhead to obtain this for all collections/views, though.

Copy link
Member

@GromNaN GromNaN Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone finds it useful, we can do it, but we'll have to check the impact on performance if there are a lot of collections.

Tracked in PHPORM-326

@masterbater masterbater changed the title feat: exclude system.* collections, add getViews feat: add getViews and categorize table types Mar 25, 2025
@masterbater
Copy link
Contributor Author

masterbater commented Mar 26, 2025

This is now a simple PR, separating tables(collections in mongodb) and views(standard views). No more segregating SYSTEM collections.

For dropAllTables, I followed the recommendation to drop the entire database instead of deleting collections individually. However, this approach may lead to errors, such as "database is in the process of being dropped." While I’m unsure if tthere any production workloads that could affect this, it does cause issues in parallel testing when using the same database—an incorrect setup that should instead use separate databases for each test.

@jmikola
Copy link
Member

jmikola commented Mar 26, 2025

For dropAllTables, I followed the recommendation to drop the entire database instead of deleting collections individually. However, this approach may lead to errors, such as "database is in the process of being dropped." While I’m unsure if tthere any production workloads that could affect this, it does cause issues in parallel testing when using the same database—an incorrect setup that should instead use separate databases for each test.

If the library needs to work around this, it might be sufficient to catch a ServerException and no-op for DatabaseDropPending code 215.

I reckon that the original approach to delete collections individually would still be subject to a race condition if a collection was created by another connection between dropAllTables() listing the collections and dropping them individually.

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GromNaN: Let me know if there's anything else for me to review, but I think you can take it from here.

@GromNaN GromNaN added this to the 5.4 milestone Apr 17, 2025
@GromNaN GromNaN changed the title feat: add getViews and categorize table types PHPORM-325 Add getViews and categorize table types Apr 18, 2025
@GromNaN GromNaN merged commit bcf97d2 into mongodb:5.x Apr 18, 2025
71 checks passed
@GromNaN
Copy link
Member

GromNaN commented Apr 18, 2025

Thank you @masterbater

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exclude System Collections in listCollectionNames()
3 participants